Skip to content

Mesh loaders kevin #199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Mesh loaders kevin #199

wants to merge 8 commits into from

Conversation

kevyuu
Copy link

@kevyuu kevyuu commented Jun 25, 2025

Fix example 9, 67, 71

@devshgraphicsprogramming devshgraphicsprogramming changed the base branch from master to mesh_loaders June 25, 2025 14:09
Base automatically changed from mesh_loaders to master July 1, 2025 08:54
Comment on lines 33 to +35
const uint64_t vertexBufferAddress = geom.vertexBufferAddress;
const uint64_t indexBufferAddress = geom.indexBufferAddress;
const uint64_t normalBufferAddress = geom.normalBufferAddress;
Copy link
Member

@devshgraphicsprogramming devshgraphicsprogramming Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are you handling calculating smooth vertex normals vs cross product from positions


uint32_t vertexStride : 29;
uint32_t objType : 29;
uint32_t indexType : 2; // 16 bit, 32 bit or none
uint32_t smoothNormals : 1; // flat for cube, rectangle, disk

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be inferred from normalBufferAddress!=0ull


uint32_t vertexStride : 29;
uint32_t objType : 29;
uint32_t indexType : 2; // 16 bit, 32 bit or none

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none should be inferred from indexBufferAddress==0 so you only need 1 bit

OT_UNKNOWN = std::numeric_limits<uint8_t>::max()
};

static constexpr uint32_t s_smoothNormals[OT_COUNT] = { 0, 1, 1, 0, 1, 1 };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can deduce from the Geometry not having a normal view

Comment on lines +35 to +61
struct ObjectMeta
{
ObjectType type = OT_UNKNOWN;
std::string_view name = "Unknown";
};

struct ObjectDrawHookCpu
{
nbl::core::matrix3x4SIMD model;
ObjectMeta meta;
};

enum GeometryShader
{
GP_BASIC = 0,
GP_CONE,
GP_ICO,

GP_COUNT
};

struct ReferenceObjectCpu
{
ObjectMeta meta;
core::matrix3x4SIMD transform;
core::smart_refctd_ptr<ICPUPolygonGeometry> data;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean this up, there's no geometry shader, the model and transform matrices are probably not used

Comment on lines +65 to +79
struct Bindings
{
nbl::asset::SBufferBinding<IGPUBuffer> vertex, index;
};

ObjectMeta meta;
Bindings bindings;
uint32_t vertexStride;
nbl::asset::E_INDEX_TYPE indexType = nbl::asset::E_INDEX_TYPE::EIT_UNKNOWN;
uint32_t indexCount = {};

const bool useIndex() const
{
return bindings.index.buffer && (indexType != E_INDEX_TYPE::EIT_UNKNOWN);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lots unused stuff, or things which can be deduces from IGPUPolygonGeometry

Comment on lines +95 to 98
uint64_t normalBufferAddress;

uint32_t vertexStride : 26;
uint32_t objType: 3;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, you've got indexType using 2 bits and smoothNormals where info can be deduced from index and normal BDA being 0 or not

@devshgraphicsprogramming
Copy link
Member

btw remove EXCLUDE_FROM_ALL in CMakeLists.txt for examples you've fixed (67 and 71) so that CI tests them now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants